-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
General interface #99
Conversation
This is partially replacing \loadModule which didn't distinguish between libraries and modules. Having \useLibrary is slightly more verbose (as one needs two commands now, but also more explicit. The new thing is that \useLibrary can take options that are directly passed to the library.
For loading libraries \useLibrary is to be used now. This is more powerful and explicit/idiomatic. Commands to load individual items or collections from a library will be added later.
Function that has to be used in a library's __init__ file. (Currently it is not enforced but \useLibrary will be updated to enforce that by performing checks for existing options). \declareLibrary is used to set a number of meta options for a library that can be used for documentation and versioning purposes
The mandatory library option maintainers is now oll-maintainers? This is either a string of name <email> or a list thereof.
version string with three integer elements
Short description (subheading) as \markup.
'short-description (as subheading) 'description (as introduction) Both as string options.
A list of known library options is used to type-check known options. The library can still add arbitrary options, but it is ensured that "known" options have a given type. That's necessary to be able to post-process them (e.g. in documentation generation)..
Now all functions in os-path that take paths as arguments accept - strings (OS independent) - lists of strings - lists of symbols (strings and symbols could be mixed as they are converted one by one.)
It is a common operation to extract the cadr and caddr from all elements of a ly:context-mod? argument. Therefore it is factored in a dedicated function.
It seems that when using gulp-file the location argument is not passed correctly. Therefore I change it to the "\include" idiom.
New interface to using modules, using a dot-symbol-path as argument. Throws a warning when the library hasn't been loaded already. Accepts an optional \with {} clause with options that are passed to the module.
\loadModule should not be used anymore.
These are needed for the new loading mechanism
I could not find a \declareLibrary "GridLY" \with {
maintainers = #'("Matteo Ceccarello <[email protected]>")
version = "0.4.0"
short-description = \markup {
GridLY is a library that implements a simple "segmented grid" approach
}
} is this the intended way of using it? I like this approach, I think this is the right direction, since it allows for more checks than simple comments. Maybe it's a little too early to have the full benefits of explicit versioning (like for expressing dependencies between libraries). However, I think that declaring the version is useful because:
Anyway, for me your implementation is OK, but since this is a fundamental change in the infrastructure I think that it would be good to hear the opinion of the others too. |
Oh, I realized too late that I hadn't pushed everything, sorry. I continued today and went in a very promising direction. But in the end I ran into very strange issues with regard to including files through I'll come back to this with comments and pushes later tonight. |
Yes, that is about the way
I think the set of mandatory and recognized options should be designed so that it makes a kind of "introductory interface" to a library. In addition to A module can be addressed with a dot-path that is matched against the directory structure. The command looks for either This seems to work very well. But when trying to remove the remaining This is where I've currently got stuck with. Regarding the comments about the versioning I fully agree. It's good to have it right from the start, but not trying to push it too far at the moment. |
OK, I'm fine with all these changes, so once you solve that issue with loadModule invocation (I'll try to have a look at it) we can merge this. Anyway, as I've said, I'd like to know the opinion of the others too before the merge. Just one observation:
I'd prefer the Python/string solution, since those fields will used for documentation on Python land for sure. |
Am 15. März 2015 16:50:34 MEZ, schrieb Matteo Ceccarello [email protected]:
U'm into something else right now but I'll ask more people about their opinion before merging.
Ok, sounds reasonable. That also opens up some Markdown options. |
Now it works. However, there's one peculiar thing: When defining a variable in __init__.ily this isn't available in __main__.ily afterwards.
It is no use exporting this to a separate file as the includes from all the different input files have been removed.
OK; I've added some more commits, updated the usage-examples and unit-tests to the new calling syntax and sorted several things out. I am still sitting on one annoying bug but have at least found a workaround so everything compiles for now. Please @PaulMorris, @PeterBjuhr, @davidnalesnik, @janek-warchol and @jpvoigt have a look at this, giving me your opinion on the new interface, and possibly give me a helping hand with the bug. For getting an idea about the interface you can read this Wiki page (only this one is up-to-date, particularly the "Common functionality" page would be important to update too) and iterate through the The bug is with the function Apart from this (critical) bug I'm very happy with the changes and the potential it has for simplifying our life with using extensions for LilyPond, so I would really like to be able to merge this ASAP. |
I have narrowed it down somewhat more: The bug is related to the dot-list invocation and the a) there are very different error messages depending on what comes next after the
This already led me to think that the command somehow stumbles over the next item and can't distinguish it from an additional list element. But the proof is that when entering the argument in standard list notation \useModule #'(scholarly diplomatic-line-breaks)
% instead of
\useModule scholarly.diplomatic-line-breaks everything works as expected. Unfortunately I can't easily reproduce this behaviour with a minimal example, but it has to be related to this dot-list parsing somehow. Any ideas? |
For example the following works as expected, and I can't see where \version "2.19.16"
testDotList =
#(define-void-function (parser location opts path)
((ly:context-mod?) list?)
(for-each
(lambda (p)
(display p)
(newline))
path))
\testDotList path.to.whatever
\testDotList \with {
optionOne = ##t
optionTwo = ##f
}
path.to.something.else
\relative { c' } |
I merge this branch, as it has turned out that the issue we discussed is actually a bug with LilyPond's parser and not with my code. There is a workaround available, and I documented the to-be-used syntax on the Wiki ages.
This is a start to implement discussions done in #86 (see my last comment (the 10th) there.